-
-
Notifications
You must be signed in to change notification settings - Fork 8.6k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
feat(v2): hide navbar on scroll #2055
Conversation
Deploy preview for docusaurus-2 ready! Built with commit e7db3f5 |
Deploy preview for docusaurus-preview ready! Built with commit e7db3f5 |
Hmm, for several days there has been no review from anyone, I think this is a good addition, don't you like it? |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think this is a useful addition, but I would classify it under "nice-to-have". Do you think it's possible to extract out all the new additions into a hook called useHideableNavbar
?
Approved. Feel free to self-merge.
packages/docusaurus-theme-classic/src/theme/Navbar/styles.module.css
Outdated
Show resolved
Hide resolved
packages/docusaurus-theme-classic/src/theme/Navbar/styles.module.css
Outdated
Show resolved
Hide resolved
Docs needed |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Friendly ping for docs & other refactor as suggested. Gonna skip it for this release
Sorry for the delay, all proposals implemented (hook, docs). |
@@ -90,6 +90,23 @@ module.exports = { | |||
|
|||
Outbound links automatically get `target="_blank" rel="noopener noreferrer"`. | |||
|
|||
### Auto-hide sticky navbar | |||
|
|||
You can enable this cool UI feature that automatically hides the navbar when a user starts scrolling down the page, and show it again when the user scrolls up. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
it feels a bit weird that here we first address reader by "you" then later on say "a user" or "the user". I think it is ok to keep using you.
just personal opinion: i prefer it to be less objective. "cool" is also a very vague adjective and i found it a bit weird to read this from our docs
* LICENSE file in the root directory of this source tree. | ||
*/ | ||
|
||
import React, {useState, useCallback, useEffect} from 'react'; // eslint-disable-line no-unused-vars |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
why need eslint disable ? is it React :O
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM
Motivation
I propose to consider including in my opinion a good feature called "Auto-Hide Sticky Header". The bottom line is simple - navbar hides when scrolling down, and is shown again when scrolling up. This helps the user to better focus on the content. Since not everyone may like it, this option is enabled in the theme config.
Have you read the Contributing Guidelines on pull requests?
Yes
Test Plan
The
hideOnScroll
option must be enabled.